Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(nginx): add ipFamilies option for IPv4 and IPv6 #639

Merged
merged 1 commit into from
Oct 1, 2024
Merged

feat(nginx): add ipFamilies option for IPv4 and IPv6 #639

merged 1 commit into from
Oct 1, 2024

Conversation

mdallaire
Copy link
Contributor

@mdallaire mdallaire commented Sep 25, 2024 •
edited
Loading

Description of the change

Add an option to configure the listening IP address for the nginx container.

Benefits

This will allow setting the IP to a value like "[::]" for IPv6 deployments. The current generated configs force the address to 0.0.0.0

Possible drawbacks

None that I can think of. The default value is 0.0.0.0 which is equivalent to the configuration generated by the chart at the moment.

Applicable issues

Additional information

Checklist

@jessebot jessebot added the 3. to review Waiting for reviews label Sep 26, 2024
@wrenix
Copy link
Collaborator

wrenix commented Sep 28, 2024 •
edited
Loading

should we add an array of multiple adresses (dualstack) or listen just on both (ipv4 and ipv6)

@provokateurin
Copy link
Member

Array sounds like a good idea, if we add it before merging this it also doesn't result in a breaking change.
As for always listening on both, I don't know if that is a good idea. If ipv6 is disabled in the kernel the container will always fail.
So array with both ipv4 and ipv6 by default would make sense to me, then it can still be reduced to ipv4 only if necessary.

provokateurin
provokateurin requested changes Sep 28, 2024
View reviewed changes
Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking for now

@mdallaire
Copy link
Contributor Author

That would be achieved with the ipv6only nginx option. I can add it to the chart.

@mdallaire
Copy link
Contributor Author

It could also be a single option similar to the ipFamilies for services. This array with possible values of IPv4 and IPv6 or both would decide how both listening address and ipv6only is built in the configuration template.

Let me know how you prefer this be done and I will adjust the PR.

provokateurin
provokateurin requested changes Sep 30, 2024
View reviewed changes
charts/nextcloud/README.md Outdated Show resolved Hide resolved
charts/nextcloud/values.yaml Outdated Show resolved Hide resolved
charts/nextcloud/files/nginx.config.tpl Outdated Show resolved Hide resolved
@mdallaire mdallaire changed the title feat(nginx): add listening address option feat(nginx): add ipFamilies option for IPv4 and IPv6 Sep 30, 2024
provokateurin
provokateurin requested changes Sep 30, 2024
View reviewed changes
charts/nextcloud/README.md Outdated Show resolved Hide resolved
provokateurin
provokateurin approved these changes Sep 30, 2024
View reviewed changes
Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good from my side, but haven't tested it. @jessebot @wrenix can one of you also review?

@provokateurin
Copy link
Member

(A squash of all commits would be nice to have)

@mdallaire
Copy link
Contributor Author

(A squash of all commits would be nice to have)

Done!

wrenix
wrenix approved these changes Oct 1, 2024
View reviewed changes
Copy link
Collaborator

@wrenix wrenix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine with it.

I am just thinking of dualstack, is not everywhere automatically on if listen [::]:80;.

Maybe we should define listen multiple time.I am not sure.
Maybe we should merge this state and wait for an Bug-Issue if somebody try to run it on a "tweaked" Node and Kernel with special option on the behaviour of binding.

@provokateurin
Copy link
Member

Agreed, we can easily fix this if there is a problem.

@mdallaire
Copy link
Contributor Author

I am just thinking of dualstack, is not everywhere automatically on if listen [::]:80;.

When listen [::]:80; is set, nginx will only listen to IPv6 (the default value for the ipv6only option is on ref). This is why we have to set listen [::]:80 ipv6only=off; when both IPv4 and IPv6 are present in the array.

@provokateurin provokateurin merged commit f184134 into nextcloud:main Oct 1, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jessebot jessebot jessebot approved these changes

@provokateurin provokateurin provokateurin approved these changes

@wrenix wrenix wrenix approved these changes

Assignees
No one assigned
Labels
3. to review Waiting for reviews
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Nginx container will not listen on IPv6
4 participants
@mdallaire @wrenix @provokateurin @jessebot